Fix handling of overlapping paths in resteasy-reactive server #50384
Conversation
TomasHofman
changed the title
quarkus-bot
bot
added
area/agroal
area/amazon-lambda
area/arc
geoand
removed
area/google-cloud-functions
area/codestarts
area/cli
|
Thanks @TomasHofman! Did this pass CI on your fork? |
|
Hi @geoand , I was locally running tests in following two modules:
and those did pass. I was hoping to get more coverage here on the PR. Now as I reread the contributors file I will get GH actions working on my fork and let you know 👍 (it's been few years since I last contributed here). |
|
@geoand actually the CI on my fork had run, and looks green (if you can access here? https://github.com/TomasHofman/quarkus/actions/runs/18191107425) |
| .body(equalTo("Foo bar_value")); | ||
| } | ||
|
|
||
| // TODO: remove this test before commit |
There was a problem hiding this comment.
I assume this need to be removed :)
|
Cool! In that case, feel free to take it out of draft when you are done |
|
@geoand sure, I will take bit more time, I'm looking at one more touch point. |
|
Thanks! No rush |
This solves further problems in case when the paths are spread over multiple resource classes. Improves earlier PR: quarkusio#47386
| } | ||
|
|
||
| return -1; | ||
| return 0; |
There was a problem hiding this comment.
Should have added a comment here... This last return statement represents a case when the current match is not found in the list of new initial matches. In which case we want to search those new matches from index 0. The -1 would mean that no more matches will be tested.
|
@geoand I think I made some progress - I added a second commit and waiting for CI results. The second commit tries to improve previous PR #47386, which kinda got in my way. It handles similar case to the current issue, but the overlapping paths are spread over multiple resource classes. (The test case added in that PR passes, but can be easily broken just by adding one more segment to the resource paths. So the fix is not 100% correct in that sense.) I was originally hoping to replace that PR by something simpler, as I thought it's little too complex and not following the previous design, but I was not able to figure out simpler solution yet. Looking from the outsider point of view, it looks to me that the code as written originally by Stuart did not really expect people to use all these overlapping paths across resource classes and methods. It just picked an "initial match" of seemingly best matching resource class (if I understand correctly what the "initial match" is about), and then picked seemingly best matching method path and tried to evaluate that, without doing any backtracking in case the pick was not eventually working. Now all these fixes are making it quite bit more complex. Where I'm heading we could say to developers "don't use the overlapping paths" :) and keep things simpler, or keep increasing the complexity to accommodate devs. And maybe one day redesign when it gets too bad... Anyway, sorry for rumbling. I will wait for the CI results and signal here when they are available. Then leave it to your judgment if to include both commits or maybe just the first. |
This improves the path matching logic in resteasy-reactive server. It fixes the case when there are overlapping path templates registered, such as:
and the incoming request has path
/path/foo/bar_something. The expectation is that this path should be matched by the first template. The current behavior however is that server returns 404.